Skip to content

Sentry Metrics and Logging#1171

Merged
pooleycodes merged 4 commits intomainfrom
2432-initial-update-to-provide-to-use-metrics-and-logging-allowing-initial-dashboard-creation
Apr 8, 2026
Merged

Sentry Metrics and Logging#1171
pooleycodes merged 4 commits intomainfrom
2432-initial-update-to-provide-to-use-metrics-and-logging-allowing-initial-dashboard-creation

Conversation

@pooleycodes
Copy link
Copy Markdown
Contributor

@pooleycodes pooleycodes commented Apr 7, 2026

Description

Sentry improvements

Packages

  • @sentry/node + @sentry/profiling-node upgraded to v10.47.0

Sentry init (sentry.js)

  • Moved Sentry.init() to module top-level + added --import to all start scripts in package.json (ESM instrumentation requirement)
  • Added enableLogs: true
  • Added beforeSend to suppress async_handled_processing_error tagged events from becoming issues

Winston transport (logger.js)

  • Added SentryTransport forwarding warn/error logs to Sentry log ingestion

URL submission metrics (submitUrlController.js)

Metric Trigger
url_submission.begun Every submission attempt
url_submission.validation_failure Pre/post validation failures (failure_type attribute)
url_submission.head_request_error HEAD request failures (reason attribute)
url_submission.async_request_failure Can't reach async service
url_submission.accepted Passed validation + sent to async
  • All failure paths also emit logger.warn with event: url_submission_failure + submittedUrl for Sentry Logs widget

Results metrics (resultsController.js)

Metric Trigger
url_submission.success Async completed with no errors
url_submission.async_processing_failure Async returned an error (error_message attribute)

Dashboard funnel

begun → accepted → success
Success rate = success / begun (formula widget)

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

If possible run in local, using a local sentry env set in .env file, then run through some test submissions to see outputs in the various dashboards created for the provide service and can see the logging pass through.

Added/updated tests?

We encourage you to keep the code coverage percentage at 80% and above.

  • Yes
  • No, and this is why: It's not functional changes, just to help better metric and logging visibility
  • I need help with writing tests

Summary by CodeRabbit

  • Chores
    • Improved error monitoring and telemetry across the app, adding more granular submission, validation, query and platform API metrics.
    • Enhanced logging: warnings/errors are now forwarded into the monitoring system for better observability.
    • Monitoring is now initialised earlier and driven by environment configuration at startup.
    • Development tooling manifest updated (esbuild) and start scripts adjusted to ensure monitoring loads on launch.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 7, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8c576563-9598-4b65-a727-d1afce7b8684

📥 Commits

Reviewing files that changed from the base of the PR and between 4209418 and df33a5b.

📒 Files selected for processing (1)
  • src/controllers/submitUrlController.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/controllers/submitUrlController.js

Walkthrough

Sentry is integrated broadly: package scripts load a Sentry setup module, Sentry packages are upgraded, Sentry is initialised at module load using dotenv, controllers and services emit Sentry metrics on success/failure, and the logger adds a Winston transport that forwards warn/error logs to Sentry.

Changes

Cohort / File(s) Summary
Dependency & startup
package.json
Start scripts changed to preload ./src/serverSetup/sentry.js via Node --import; @sentry/node and @sentry/profiling-node bumped to ^10.47.0; esbuild added to devDependencies.
Sentry initialisation
src/serverSetup/sentry.js
Adds dotenv.config(); moves Sentry.init to top-level guarded by SENTRY_ENABLED; setupSentry(app) now only sets up Express error handler when enabled.
Controllers — submission & results
src/controllers/submitUrlController.js, src/controllers/resultsController.js
Added Sentry metrics across submission lifecycle and async processing: counters for begun/accepted/validation/head/request failures/async processing failures; structured metric attributes and tags for handled async errors; minor refactor of URL normalization and validation flow.
Services — errors instrumentation
src/services/datasette.js, src/services/platformApi.js
On caught errors, increment Sentry metrics (datasette_query_errors, platform_api_errors) with request/query URLs as attributes; logging payloads include resolved query URLs.
Logging integration
src/utils/logger.js
Introduces a SentryTransport for Winston that forwards warn/error logs to Sentry (non-test environments) alongside existing transports.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • eveleighoj

Poem

🐰 I hop through code with eager paws,

I stitch in metrics, mend the flaws,
Errors caught and counted neat,
Logs and traces all meet,
Carrots of insight — crisp and sweet 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'Sentry Metrics and Logging' accurately reflects the main objective: adding Sentry metrics and logging instrumentation throughout the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2432-initial-update-to-provide-to-use-metrics-and-logging-allowing-initial-dashboard-creation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 65.95% 6885 / 10439
🔵 Statements 65.95% 6885 / 10439
🔵 Functions 63.53% 277 / 436
🔵 Branches 77.95% 916 / 1175
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/controllers/resultsController.js 56.81% 68.29% 50% 56.81% 21-40, 67-71, 75-92, 100-101, 103-104, 108-109, 146-148, 178-186, 198-199, 236-237, 246-254, 257-260, 263-282, 295-319, 329-330, 357-359, 375-381, 389-404, 411-432, 444-454, 457-459, 468-481, 523-542
src/controllers/submitUrlController.js 75.95% 72.91% 94.44% 75.95% 42-46, 93-95, 140-180, 202-204, 214-215, 221-223, 239-241, 256-258
src/serverSetup/sentry.js 0% 0% 0% 0% 1-28
src/services/datasette.js 100% 100% 100% 100%
src/services/platformApi.js 70.7% 53.33% 40% 70.7% 27-28, 65-85, 99-105, 109-124
src/utils/logger.js 85% 83.33% 100% 85% 7-11, 16
Generated in workflow #1376 for commit df33a5b by the Vitest Coverage Report Action

…s-and-logging-allowing-initial-dashboard-creation
@pooleycodes pooleycodes marked this pull request as ready for review April 7, 2026 10:20
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
src/utils/logger.js (1)

13-25: Prefer Sentry's Winston transport helper here.

The SDK already ships createSentryWinstonTransport(Transport), so carrying a bespoke transport just adds maintenance surface for a first-party integration. Winston's own transport guidance also treats winston-transport as the base contract for custom transports. (github.com)

♻️ Proposed refactor
-class SentryTransport extends Transport {
-  log (info, callback) {
-    if (info.level === 'warn') Sentry.logger.warn(info.message, info)
-    else if (info.level === 'error') Sentry.logger.error(info.message, info)
-    callback()
-  }
-}
+const SentryTransport = Sentry.createSentryWinstonTransport(Transport)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/utils/logger.js` around lines 13 - 25, Replace the custom SentryTransport
with Sentry's provided helper: remove the SentryTransport class and use
createSentryWinstonTransport (or createSentryWinstonTransport(Transport) from
Sentry SDK) when building appTransports so the Sentry transport is instantiated
and configured by the SDK; update appTransports to include new
createSentryWinstonTransport({ level: 'warn' }) (or equivalent factory call)
alongside transports.Console and transports.File and ensure any options
previously passed into SentryTransport (e.g., level) are forwarded to the SDK
factory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/controllers/resultsController.js`:
- Around line 82-88: The current branch emits the raw errMsg to Sentry metrics
and only sets Sentry.getCurrentScope().setTag('async_handled_processing_error',
true) in the non-empty branch; instead normalize errMsg into a bounded failure
category (e.g., map error text to a fixed code or type) when calling
Sentry.metrics.count('url_submission.async_processing_failure', 1, { attributes:
{ error_category: ... } }) and ensure suppression is applied consistently by
either setting the 'async_handled_processing_error' tag in both branches or,
better, mark the created MiddlewareError instance itself (e.g., a
handledAsyncProcessing flag/property passed in the third-argument metadata) so
suppression logic can read that marker rather than mutating getCurrentScope().

In `@src/controllers/submitUrlController.js`:
- Around line 42-43: The logs in submitUrlController.js currently send the raw
user-supplied submittedUrl to external sinks (via logger.warn/logger.error and
Sentry), so replace the raw URL with a scrubbed value before logging: add or use
a helper (e.g., redactUrl(url) or hashUrl(url) in utils) that either strips
query strings/sensitive fragments or returns a stable hash/origin+path, then
update the logger.warn/logger.error calls (and any Sentry-related log
attributes) in the submit handler (the submit URL controller where logger.warn
is called) and at the other noted call sites (lines referenced 64-65, 79-80,
86-87, 93-94) to pass the scrubbed value instead of submittedUrl; ensure the
helper is deterministic and does not retain query parameters or sensitive
tokens.
- Around line 91-95: postCheckFailure branch logs omit the funnel event field;
update the failure logging so the logger.warn call in the postCheckFailure
handling includes event: 'url_submission_failure' (the Sentry.metrics.count line
can remain unchanged). Locate the postCheckFailure =
postValidators(headResponse).find(...) and add the event property to the
logger.warn payload (preserve existing keys failure_type, type, submittedUrl) so
exists/restricted403/size failures are recorded under the expected event key.

In `@src/services/datasette.js`:
- Around line 29-30: The metric and log currently use the full Datasette URL
(variable url) which contains encoded SQL and creates high-cardinality metric
labels; update Sentry.metrics.count call to remove url from attributes and
instead use stable dimensions such as database and a short query category/name
(e.g. derive a small queryName from the query or pass queryCategory) and modify
logger.warn (in the runQuery()/runQuery function context) to avoid embedding the
full url in metric attributes—keep url only in the warning message or
strip/obfuscate SQL if you must log it, and ensure
Sentry.metrics.count('datasette_query_errors', 1, { attributes: { database,
queryName } }) is used instead of including url.

In `@src/services/platformApi.js`:
- Line 147: The Sentry.metrics.count call currently tags the counter with the
full request URL (Sentry.metrics.count('platform_api_errors', 1, { attributes: {
url } })), which creates unbounded time series; change it to use bounded
attributes instead (e.g., derive and pass endpoint name/path template, HTTP
status family like 5xx/4xx, and a short failure reason code) so the metric uses
fixed cardinality labels; update the call site in platformApi.js to replace the
url attribute with those bounded attributes (endpoint, statusFamily,
failureReason) while keeping the metric name the same.

---

Nitpick comments:
In `@src/utils/logger.js`:
- Around line 13-25: Replace the custom SentryTransport with Sentry's provided
helper: remove the SentryTransport class and use createSentryWinstonTransport
(or createSentryWinstonTransport(Transport) from Sentry SDK) when building
appTransports so the Sentry transport is instantiated and configured by the SDK;
update appTransports to include new createSentryWinstonTransport({ level: 'warn'
}) (or equivalent factory call) alongside transports.Console and transports.File
and ensure any options previously passed into SentryTransport (e.g., level) are
forwarded to the SDK factory.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 29eb08f7-f21d-4289-9769-527577c0b1e5

📥 Commits

Reviewing files that changed from the base of the PR and between 2b0d00f and 4209418.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (7)
  • package.json
  • src/controllers/resultsController.js
  • src/controllers/submitUrlController.js
  • src/serverSetup/sentry.js
  • src/services/datasette.js
  • src/services/platformApi.js
  • src/utils/logger.js

…ng-allowing-initial-dashboard-creation' of https://github.com/digital-land/submit into 2432-initial-update-to-provide-to-use-metrics-and-logging-allowing-initial-dashboard-creation
Copy link
Copy Markdown
Contributor

@gibahjoe gibahjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pooleycodes pooleycodes merged commit a17f2ec into main Apr 8, 2026
5 checks passed
@pooleycodes pooleycodes deleted the 2432-initial-update-to-provide-to-use-metrics-and-logging-allowing-initial-dashboard-creation branch April 8, 2026 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Initial Update to Provide to use Metrics and Logging allowing initial Dashboard Creation

2 participants